Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lfs_vfs:Use provided lfs_getattr / lfs_setattr instead of lfs_file_getattr / lfs_ file_setattr #14901

Closed
wants to merge 1 commit into from
Closed

lfs_vfs:Use provided lfs_getattr / lfs_setattr instead of lfs_file_getattr / lfs_ file_setattr #14901

wants to merge 1 commit into from

Conversation

crafcat7
Copy link
Contributor

Summary

This change is still under discussion:
#14844
#14479
Before the littlefs community merges lfs_file_getattr / lfs_file_setattr, use the existing lfs_setattr / lfs_getattr to achieve the same purpose.
This change may be relatively cumbersome because we need to convert from filep to path.

Changes
The implementation is as summarized above

Impact

Is a new feature added?: NO
Impact on build: NO
Impact on hardware: NO, this change does not specifically target any particular hardware architectures or boards.
Impact on documentation: NO,This patch does not introduce any new features
Impact on compatibility: This implementation aims to be backward compatible. No existing functionality is expected to be broken.

Testing

Build Host(s): Linux x86
Target(s): sim/nsh

It is insensitive to the use of the upper layer. We use another interface to implement the equivalent method.

nsh> cd test
nsh> hello
File size: 5 bytes
File permissions: 777
Last access time: 0
Last modification time: 0

@crafcat7
lfs_vfs:Use provided lfs_getattr / lfs_setattr instead of lfs_file_ge…
2438f35 
…tattr / lfs_ file_setattr

Summary:
  We can replace lfs_file_xxxattr with lfs_getattr / lfs_setattr already integrated into littlefs

Signed-off-by: chenrun1 <[email protected]>
@github-actions github-actions bot added Area: File System File System issues Size: S The size of the change in this PR is small labels Nov 22, 2024
@crafcat7 crafcat7 mentioned this pull request Nov 22, 2024
Open
@nuttxpr
Copy link

nuttxpr commented Nov 22, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements. While it addresses most sections, it has critical omissions and weaknesses:

  • Summary: Lacks clarity on why this workaround is necessary beyond the pending upstream merges. What specific problem does this solve now? "Relatively cumbersome" is vague. Explain the drawbacks of this approach clearly. The "Changes" section just repeats the summary. Detail how the conversion from filep to path is done. This is crucial information missing from the description.

  • Impact: While claiming no impact on users, the summary mentions cumbersome conversion. This implies potential performance implications, which should be addressed. Even if minor, it's an impact. Justify the NO impact claims more thoroughly. Is there no change in code size or execution time?

  • Testing: The provided testing is extremely weak. Simply showing "hello" and some file attributes proves almost nothing. Demonstrate specific scenarios where the previous implementation failed or was deficient and how this change addresses those. Provide more comprehensive logs, including the actual commands used and their full output. Testing only on sim/nsh is insufficient. Test on real hardware if possible. Crucially, show the "before change" logs to illustrate the problem this PR solves. Currently, there's no basis for comparison.

Specific Recommendations:

  1. Elaborate on the "why": Explain the concrete problem this PR solves in the current NuttX version due to the lack of lfs_file_getattr/lfs_file_setattr. What breaks or is deficient without this change?

  2. Detail the "how": Explain the filep to path conversion mechanism precisely. Include code snippets if necessary. What are the performance implications of this conversion?

  3. Strengthen the "Impact" assessment: Address all potential impacts, even if minor. Justify "NO" answers with concrete reasons. Don't dismiss potential performance impacts.

  4. Provide comprehensive testing:

    • Show "before" logs demonstrating the problem.
    • Show "after" logs demonstrating the fix.
    • Test edge cases and error scenarios.
    • Test on more than just sim/nsh. Target real hardware if feasible.
    • Include the exact commands used and their complete output.

By addressing these points, the PR will better meet NuttX requirements and be more likely to be accepted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

@yamt yamt Awaiting requested review from yamt

Assignees
No one assigned
Labels
Area: File System File System issues Size: S The size of the change in this PR is small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@crafcat7 @nuttxpr @yamt @xiaoxiang781216